fix(catalog): close the five catalog-indexing findings (#175) - #204

…er (#175)

Boarded 2 AGREE / 1 DISAGREE.

Item 3 would have been a security regression. PolicyManager._load_policy with
fatal=False swallows any validation error and leaves the default allow-all
GatewayPolicy in place, so adding Field(ge=1) would make every policy file
containing max_tools_per_server: 0 schema-invalid and silently discard that file
whole -- allow/deny lists, limits and redaction all reverting to permissive
defaults. Verified in policy.py:59-77. The plan now fixes the misleading log
instead, and the underlying fail-open is filed as #202 rather than folded in.

Item 4 named _required_identity, which requires a non-empty str; inputSchema is
an object, so a literal call would have rejected every valid tool. Now a sibling
required-object check. Six existing fixtures in tests/mcp2x/test_catalog_
publishers.py index a tool with no inputSchema and assert == 1; they are on the
change list rather than left to surface as an apparent bug in items 1 and 5.

The item 5 criterion compared all three _index_* results against self._tools, so
it could not prove the guarantee for resources or prompts. Now per-catalog.

Also removed a 98-server manifest check that was npm identity resolution, not
catalog indexing, and was false reassurance here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3

Three spots still described the superseded design: item 4's bullet named
_required_identity two paragraphs above the note saying not to use it, the test
list still asserted LimitsPolicy(0) raises, and Documentation impact still
referenced a ge=1 bound the plan no longer adds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3

Item 1 of #175, and the one with teeth. Mutating the truncation guard
`len(entries) >= limit` to `> limit` survived this entire test file -- it
was the sole survivor of nine mutants -- because nothing asserted how many
tools land when a server offers more than `max_tools_per_server`.

Written and committed against unchanged code, so the mutation proof that
follows is a proof and not a coincidence: exact counts at `limit - 1`,
`limit` and `limit + 1`. Only the `limit + 1` case distinguishes `>=` from
`>`, which is why "fewer than offered" would not have done.

Every fixture tool carries an `inputSchema` -- item 4 makes a tool without
one unparseable, and a fixture missing it would report zero indexed for a
reason that has nothing to do with the limit.

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

… server

Items 3 and 5 of #175.

Item 5. `_index_resources`' docstring promised "the count returned is what
was actually indexed, not what was offered, so a caller reporting it is not
overstating the catalog" while all three `_index_*` returned `len(entries)`.
Two entries sharing an identity are two list items and one catalog key --
the second overwrites the first -- so the promise was false precisely when
identities collide, and a documented guarantee contradicted by the code is
worse than an undocumented gap, because a reader trusts it. The write and
the count now both live in `_apply_entries`, which counts distinct
identities and logs each collision at DEBUG naming the id: the honest count
makes last-write-wins visible, the log makes it diagnosable.

Item 3. `max_tools_per_server: 0` empties the parse result before a single
entry is examined, so `_reconcile_once` announced "Every tools entry in the
listing was unparseable" -- an accusation aimed at the downstream for a
decision this gateway's own policy file made. Both that message and the
parser's "has more than 0 tools, truncating" now name the limit.

Deliberately fixed in the log and NOT in the schema. `LimitsPolicy` gets no
`Field(ge=1)`: `_load_policy(..., fatal=False)` swallows validation errors
and falls back to the allow-all default, so rejecting `0` would silently
discard the operator's entire policy file -- allow and deny lists and
redaction included. That fail-open is #202 and is not this change's to fix.
A test pins that `0` still validates, so a future tightening cannot land
here unnoticed.

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

…dexing

Item 2 of #175. `_connect_stdio`, `_reconcile_once` and `_cleanup_client`
all remove a server's catalog entries before (re-)indexing it;
`adopt_process` was the odd one out. Adopting a server that had already
been indexed under the same name therefore left the previous listing's
tools in the catalog beside the new ones -- entries the adopted process
does not serve, still routable, until something unrelated removed them.

Made uniform rather than documented as an exception: three call sites
already do it, so the odd one out is the surprise, and one rule is cheaper
to hold in the head than an exception written down in two places.

The tests assert on catalog contents, not on whether the removal method was
called -- a call-count assertion passes just as happily if the removal runs
after the index. Verified with teeth: dropping the new line reds two of the
four (exit 1), restoring it greens them (exit 0).

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

Amends item 5 of #175. The first cut moved the write and the count together
into a module-level `_apply_entries(catalog, ...)` helper. That was wrong in
a way the suite caught: `tests/runtime/test_publisher_coverage.py` is an AST
honesty guard that attributes every `self._tools`/`self._resources`/
`self._prompts` write to its enclosing method and fails if one appears
outside the five publishing mutators. A helper that takes the dict as a
parameter is invisible to that walk, so the refactor did not trip the guard
-- it silently emptied it, leaving `_index_tools`, `_index_resources` and
`_index_prompts` looking like they no longer write to a catalog at all.

The write goes back where the guard can see it. The helper, now
`_distinct_indexed`, only counts and logs, which is all item 5 ever needed
it to do. Its docstring records why it must not take the catalog dict, so
the next reader does not re-make the same simplification.

No behaviour change from the previous commit: the counts and the duplicate
log are identical.

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

…nything

Item 4 of #175, and the only behaviour change in the set -- landed last and
alone so a bisect reaches it cleanly.

`tool.get("inputSchema", {})` manufactured a schema the server never sent,
and `{}` does not mean "we do not know": it means "any arguments at all are
valid", published under that server's name to every caller and every model
reading the catalog. MCP requires `inputSchema` on a tool, so a tool without
one is a tool we could not read, and #172 already settled what happens to
those -- skip it and say so, rather than invent the missing value.

The check is a new `_required_object`, a sibling of `_required_identity`
rather than a flag on it: that helper requires `isinstance(value, str) and
value`, so calling it for an object-valued field would reject every valid
tool in the catalog. It raises inside the parser's existing per-entry `try`,
so a schemaless tool costs only itself, and a listing of nothing but such
entries lands in `_reconcile_once`'s offered-but-none-parseable rule --
prior tools kept, nothing published as removed.

An explicit `inputSchema: {}` is still accepted: the server said "any
arguments", and that is an answer. Absence, `null`, and any other non-object
are not.

Fixture fallout, all of it in tests/mcp2x/test_catalog_publishers.py, at the
six lines the plan predicted (130, 140, 193, 218, 278, 310) and nowhere
else: the full suite surfaced no other schemaless tool fixture.

CHANGELOG calls this out under `### Changed`, not buried in `### Fixed`, so
a reader upgrading sees that a downstream omitting `inputSchema` loses that
tool from the catalog.

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

Board on PR #204 was 2 AGREE / 1 DISAGREE with no correctness defect found,
and one unmet acceptance criterion: the mutation evidence existed only in a
transcript. #180, #183 and #187 all carry `.consiliency/evidence/mutation-*.md`
for exactly this reason -- the only thing separating item 1's fix from the
nine-mutant battery that missed it is a recorded before/after, and a claim in
a chat message is not that.

Both mutations were re-run from scratch rather than transcribed, with exit
codes captured from the shell:

  boundary `>=` -> `>`      : 0 -> 1 -> 0, git diff empty after restore
  adopt_process line deleted: 0 -> 1 -> 0, git diff empty after restore

The file also records the one regression no mutation of the product code
would have caught -- item 5's first cut moving the catalog write into a
helper, which made it invisible to the publisher-coverage AST guard and
silently emptied that guard rather than tripping it.

Also in this commit:

* `adopt_process`'s comment. A seat reported that `_reconcile_once` does not
  remove before indexing. Checked against source: it does -- line 2203,
  `_remove_server_indexes(...)` immediately precedes the three `_index_*`
  calls inside the apply block. So the original claim was not false, but it
  was misleading, because reconcile's removal is per-kind, post-fetch and
  conditional on the listing having succeeded, which is a different rule for
  a different situation. The comment now names the connect and cleanup paths
  as the actual precedent and says explicitly that reconcile is not one. The
  line numbers are dropped: the three the comment first carried were already
  stale within this same change, which is the argument against citing them.

* The plan's verification block, scoped from `ruff format --check .` to
  `... src/ tests/`, matching CI's lint job, with a note that the two
  `diagnostics/issue-79-1b/` files are known pre-existing debt. The repo-wide
  form was over-broad and produced a false signal; reformatting untouched
  files from a catalog-indexing PR would be scope creep.

See #175.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018W2e4mGLxCfQeKqQ1P6CPB

# Conflicts:
#	CHANGELOG.md